Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update table styles so nhsuk-table has bottom margin #1005

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

edwardhorsford
Copy link
Contributor

@edwardhorsford edwardhorsford commented Sep 6, 2024

This fixes some inconsistency with the table styles.

The current table styles rely on a bottom margin being applied to the table element rather than nhsuk-table. The responsive table variant resets this so it has no bottom margin.

The styles include a nhsuk-table-container class that's presumably meant to wrap tables, but this is not used or documented on the design system site. I might guess it was added for a specific use case. For now I've left it, but teams shouldn't need to use it to get the correct bottom margin.

This applies some basic styles to nhsuk-table similar to how govuk-table styles it. If nhsuk-table-container can still be used.

I think this should probably have no visual impact for most users. But if any had added extra margin to account for the responsive table not having margin, then that could double. It now sets the font too - which it's possible could conflict with a non standard font.

@frankieroberto
Copy link
Contributor

frankieroberto commented Sep 10, 2024

Might be worth opening a second PR to remove .nhsuk-table-container - looks like it’s no longer used for responsive tables at all (those use .nhsuk-table--responsive)?

Do you know about this at all @anandamaryon1?

@anandamaryon1
Copy link
Collaborator

Looks good to me, I've not seen the .nhsuk-table-container being used anywhere but looking at the styles it could be used to create a scrolling table?

Perhaps leave it in for now.

Based on this, the responsive table example will need both classes, so the block class adding too (.nhsuk-table), for the margin bottom.

Note to self: will need to check service manual aligns.

@@ -22,6 +22,16 @@
}
}

.nhsuk-table {
@include nhsuk-font($size: 19);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for adding this? Could this perhaps mess up any specific type styling that people may have added in their tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mirrors what GOV.UK do - providing some default sizing. I'd hope it wouldn't override any specific sizing as it should be less specific than any added by users. Not 100% confident about that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants